pg: add returning_id option to parallel_execute#181
Closed
cawo-odoo wants to merge 1 commit intoodoo:masterfrom
Closed
pg: add returning_id option to parallel_execute#181cawo-odoo wants to merge 1 commit intoodoo:masterfrom
cawo-odoo wants to merge 1 commit intoodoo:masterfrom
Conversation
Contributor
|
|
cawo-odoo
force-pushed
the
master-imp_pg_add_returning_id_to_parallel_execute-cawo
branch
2 times, most recently
from
66f85ef to
5f07404
Compare
The default behavior is unchanged. This adds the possibility to parallelize modifying queries that have a `RETURNING id` clause. For those, return the resulting ids (in a defined order) instead of the affected row count. To avoid misuse add a warning to the docstring and try to detect queries other than the ones of the intended form. Raise an error if such are found.
cawo-odoo
force-pushed
the
master-imp_pg_add_returning_id_to_parallel_execute-cawo
branch
from
5f07404 to
343aa7e
Compare
Pirols
reviewed
Jan 2, 2025
Contributor
Pirols
left a comment
Pirols
left a comment
There was a problem hiding this comment.
I know it's a draft, couldn't help peeking.
Could you further elaborate on the need to limit this to returning ids only?
| :param list(str) queries: list of queries to execute concurrently | ||
| :param `~logging.Logger` logger: logger used to report the progress | ||
| :return: the sum of `cr.rowcount` for each query run | ||
| :param bool returning_id: wether to return a tuple of affected ids (default: return affected row count) |
Contributor
There was a problem hiding this comment.
Suggested change
| :param bool returning_id: wether to return a tuple of affected ids (default: return affected row count) | |
| :param bool returning_id: whether to return a tuple of affected ids (default: return affected row count) |
| - As a side effect, the cursor will be committed. | ||
| - It would not be generally safe to use this function for selecting queries. Because of this, | ||
| `returning_id=True` is only accepted for `UPDATE/DELETE/INSERT/MERGE [...] RETURNING id` queries. Also, the | ||
| caller cannot influnce the order of the returned result tuples, it is always sorted in ascending order. |
Contributor
There was a problem hiding this comment.
Suggested change
| caller cannot influnce the order of the returned result tuples, it is always sorted in ascending order. | |
| caller cannot influence the order of the returned result tuples, it is always sorted in ascending order. |
| raise ValueError("The returning_id parameter can only be used with certain queries.") | ||
|
|
||
| res = parallel_execute_impl(cr, queries, logger, returning_id) | ||
| return tuple(sorted([id for (id,) in res])) if returning_id else res |
Contributor
There was a problem hiding this comment.
Suggested change
| return tuple(sorted([id for (id,) in res])) if returning_id else res | |
| return tuple(id for (id,) in res) if returning_id else res |
Why sorting these?
Comment on lines
+181
to
+184
| if returning_id: | ||
| returning_id_re = re.compile(r"(?s)(?:UPDATE|DELETE|INSERT|MERGE).*RETURNING\s+\S*\.?id\s*$") | ||
| if not all((bool(returning_id_re.search(q)) for q in queries)): | ||
| raise ValueError("The returning_id parameter can only be used with certain queries.") |
Contributor
There was a problem hiding this comment.
Suggested change
| if returning_id: | |
| returning_id_re = re.compile(r"(?s)(?:UPDATE|DELETE|INSERT|MERGE).*RETURNING\s+\S*\.?id\s*$") | |
| if not all((bool(returning_id_re.search(q)) for q in queries)): | |
| raise ValueError("The returning_id parameter can only be used with certain queries.") | |
| returning_id_re = re.compile(r"(?s)(?:UPDATE|DELETE|INSERT|MERGE).*RETURNING\s+\S*\.?id\s*$") | |
| returning_id = all((bool(returning_id_re.search(q)) for q in queries)) |
How about we automatically detect whether the queries at hand are returning something?
| return parallel_execute_impl(cr, queries, logger=_logger) | ||
|
|
||
| if returning_id: | ||
| returning_id_re = re.compile(r"(?s)(?:UPDATE|DELETE|INSERT|MERGE).*RETURNING\s+\S*\.?id\s*$") |
Contributor
There was a problem hiding this comment.
Suggested change
| returning_id_re = re.compile(r"(?s)(?:UPDATE|DELETE|INSERT|MERGE).*RETURNING\s+\S*\.?id\s*$") | |
| returning_id_re = re.compile(r"(?s)(?:UPDATE|DELETE|INSERT|MERGE).*RETURNING\s+(?:\w+\.)?id\s*$") |
Change \S*\.? to (?:\w+\.)? to avoid false positive like "UPDATE ... RETURNING name,id" and "DELETE FROM ... RETURNING report_id".
Contributor
|
For the record: I think this is not necessary. See more comments in the linked PR in upgrades. |
cawo-odoo
deleted the
master-imp_pg_add_returning_id_to_parallel_execute-cawo
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The default behavior is unchanged.
This adds the possibility to parallelize modifying queries that have a
RETURNING idclause. For those, return the resulting ids (in a defined order) instead of the affected row count.To avoid misuse add a warning to the docstring and try to detect queries other than the ones of the intended form. Raise an error if such are found.